2. PNG files cannot yet be deleted, as some are referenced across bundles via hardcoded paths e.g. in platform code. Removing these files would result in errors.

We could clean up (most of) these references across bundles so that we can delete (most of) the PNGs.

Thank you @Michael5601 for this PR I'm really looking forward to this.

Withouth having it tried out yet, I have a few remarks/questions/suggestions, all based on the assumption that providing support to render vector graphics is an 'implementation' detail that does not need any or only little new APIs where technically necessary (e.g. to specify a zoom factor).

Therefore I wonder for example a new public API type ISVGRasterizer is necessary or if it should better be an internal class, just like the other new types.
I assume it's not intended to allow external contributions of custom rasterizers? And as far as I understood our discussion, on the long run SVG support could become fully built in in SWT and then we have to decide again about the way JSVG is integrated.

Splitting the steps to handle all kind of icons sounds very reasonable for me. I think this should just focus on adding support for rendering vector graphics, just as it's done.

1. Fragment Approach:
   Adding a fragment to SWT with the JSVG dependency failed, as SWT itself is composed of fragments, and fragments cannot depend on other fragments.

It's correct that Fragments can't have own Fragments, but a (host) bundle can have an arbitrary number of fragments.
So it would be possible to make o.e.swt.svg another fragment of o.e.swt and since all fragments and the their host bundle share the same classloader code from the corresponding native SWT fragments can then access classes from the svg fragment.
Then you could use the plain Java ServiceLoader without further addo or even just load a fixed class reflectivly using Class.forName(). This would avoid the need for adding a SVGRasterizerRegistry and making that the registration happens and happens early enough.

Setting up the java ServiceLoader between OSGi bundles (not fragments) on the other hand is a bit more complicated, but also possible. Just for reference, this blog post should explain it (I don't want to go into the details here):
https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html

Test Results

   382 files   -   1     382 suites   - 1   6m 13s ⏱️ + 1m 20s
 4 251 tests  -  35   4 242 ✅  -  30   8 💤  - 6  1 ❌ +1 
12 305 runs  +155  12 214 ✅ +149  90 💤 +5  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 44e6f19. ± Comparison against base commit 2a61cb2.

org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testTextTransfer
…

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_TabTraversalOutOfBrowser[browser flags: 524,288]

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_Constructor_multipleInstantiationsInDifferentThreads[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_TabTraversalOutOfBrowser[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_setUrl_local[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_setUrl_remote[browser flags: 0]

♻️ This comment has been updated with latest results.

Thank you for the comment, I will check all your remarks soon.

Setting up the java ServiceLoader between OSGi bundles (not fragments) on the other hand is a bit more complicated, but also possible. Just for reference, this blog post should explain it (I don't want to go into the details here): https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html

To this point I can already say that I tried it with the same article but I wasn't successful.

Therefore I wonder for example a new public API type ISVGRasterizer is necessary or if it should better be an internal class, just like the other new types. I assume it's not intended to allow external contributions of custom rasterizers? And as far as I understood our discussion, on the long run SVG support could become fully built in in SWT and then we have to decide again about the way JSVG is integrated.

That's something we should discuss and agree on. I would not in favor of tightly integrating a specific SVG rasterizer into SWT. Having it interchangeable ensures low coupling and gives the flexibility to exchange it without rebuilding SWT (by just providing some other fragment/bundle/... that contains an alternative rasterizer implementation). We could still think of making that interface internal for now, so that the interface is not public but in case you would want to provide a different rasterizer it would still be possible (with potentially a warning).

1. Fragment Approach:
   Adding a fragment to SWT with the JSVG dependency failed, as SWT itself is composed of fragments, and fragments cannot depend on other fragments.

It's correct that Fragments can't have own Fragments, but a (host) bundle can have an arbitrary number of fragments. So it would be possible to make o.e.swt.svg another fragment of o.e.swt and since all fragments and the their host bundle share the same classloader code from the corresponding native SWT fragments can then access classes from the svg fragment. Then you could use the plain Java ServiceLoader without further addo or even just load a fixed class reflectivly using Class.forName(). This would avoid the need for adding a SVGRasterizerRegistry and making that the registration happens and happens early enough.

For some more clarifiation on using a fragment: If we are not mistaken, to provide a fragment with a concrete rasterizer implementation, the rasterizer interface needs to be placed directly in the SWT host bundle, not in some common package that is mapped into all the native fragments like done for all the other SWT code. Otherwise, the SVG fragment would be tied to the presence of another fragment providing that interface, which (reasonably) seems to be prohibited. As a consequence, that interface would become the first source element to be placed directly in the SWT host bundle, while all the other code is placed in the native fragments. In addition, we need to reference that interface from within the native fragments (to make use a rasterizer in SWT code). With the current configuration of the bundles/fragment, this is not possible. The SWT host bundle has to be added to the classpath of the native fragments to access the interface via adding the PDE required plugins container to its classpath (i.e., to add the classpath entry <classpathentry kind="con" path="org.eclipse.pde.core.requiredPlugins"/>). All this may not be prohitibitive, but it's all changes to the current configuration of the SWT host and fragments that need to be considered.

Setting up the java ServiceLoader between OSGi bundles (not fragments) on the other hand is a bit more complicated, but also possible. Just for reference, this blog post should explain it (I don't want to go into the details here): https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html

I just want to add that I also tried to create a minimal reproducer for using a ServiceLoader across OSGi bundles, but I also failed, just like Michael. The necessary capabilities added to the manifests do not even show up when listing the capabilities of the bundles via the OSGi console. Probably, I am / we are doing something wrong here.

I edited the PR message and added the following points:

1. Planned Tasks for this PR: I will include regression tests and I will perform a performance evaluation. The feature also needs to be tested for gtk and cocoa as they have different ImageLoader implementations.
2. New Outstanding issue: There are missing or broken SVGs that do not need to be fixed for this feature to work but if we plan to remove all PNGs finally this issue needs to be resolved.

Please see this Draft for the Follow-up-functionality for the automatic customization of graphically customized icons as announced previously in this PR.

The functionality is complete but the draft is meant as a base for discussion. All changes to this PR will be done also in the draft.

Therefore I wonder for example a new public API type ISVGRasterizer is necessary or if it should better be an internal class, just like the other new types. I assume it's not intended to allow external contributions of custom rasterizers? And as far as I understood our discussion, on the long run SVG support could become fully built in in SWT and then we have to decide again about the way JSVG is integrated.

That's something we should discuss and agree on.

Yes, absolutely. I suggest we discuss this in a separate issue, in parallel to this PR.

Setting up the java ServiceLoader between OSGi bundles (not fragments) on the other hand is a bit more complicated, but also possible. Just for reference, this blog post should explain it (I don't want to go into the details here): https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html

I just want to add that I also tried to create a minimal reproducer for using a ServiceLoader across OSGi bundles, but I also failed, just like Michael. The necessary capabilities added to the manifests do not even show up when listing the capabilities of the bundles via the OSGi console. Probably, I am / we are doing something wrong here.

Besides adding the correct required and provided capabilities to the provider and consumer of the service (implementation), you also have to make sure that aries.spifly as well as the provider of the service (implementation) are auto-started at an appropriated level, see for example how it's done for slf4j-simple providing a logging-provider consumed by the slf4j-api bundle:
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/02e51912906b367d441fb0d120c7fd3bc3990dad/eclipse.platform.releng.tychoeclipsebuilder/eclipse.platform.repository/sdk.product#L193-L200

Everyone building products or launching Eclipse-apps from a workspace that include this SVG support for SWT would have to configure this. This would IMHO be cumbersome and to avoid all these difficulties I strongly would like to avoid the need for the OSGI Service Loader mediator in this case.

1. Fragment Approach:
   Adding a fragment to SWT with the JSVG dependency failed, as SWT itself is composed of fragments, and fragments cannot depend on other fragments.

It's correct that Fragments can't have own Fragments, but a (host) bundle can have an arbitrary number of fragments. So it would be possible to make o.e.swt.svg another fragment of o.e.swt and since all fragments and the their host bundle share the same classloader code from the corresponding native SWT fragments can then access classes from the svg fragment. Then you could use the plain Java ServiceLoader without further addo or even just load a fixed class reflectivly using Class.forName(). This would avoid the need for adding a SVGRasterizerRegistry and making that the registration happens and happens early enough.

For some more clarifiation on using a fragment: If we are not mistaken, to provide a fragment with a concrete rasterizer implementation, the rasterizer interface needs to be placed directly in the SWT host bundle, not in some common package that is mapped into all the native fragments like done for all the other SWT code.

That's right. The only disadvantage I can think of for having code in the o.e.swt host bundle to, would be that for plain-Java/non-OSGi apps o.e.swt would then also need o.e.swt on the classpath, while they only need the native fragment of the current platform now. So I guess that wouldn't be ideal, altough I don't know if it's forbidden by a rule.
But maybe we could make that change only for the compilation in the workspace, because at runtime because all fragments and their host share the same classloader, the actual location of a class-file among them, is irrelevant (maybe with multiple occurrences of the same class).
But it would also lead to another problem: Because SVGRasterizer returns an ImageData object that class must be moved to o.e.swt too and with it it's whole dependency-closure and eventually we would more or less revert:

• Link to java sources into native fragments to compile them in fragments #1008

But I tried a few things out and had another idea:
If o.e.swt.svg is a fragment of o.e.swt, we could simply add an explicit requirement to the native fragments to the build-path of o.e.swt.svg. See the details below in the code-remarks.
But that should be a solution that seems feasible.

Another option would be to not provide an interface SVGRasterizer at all and just load the class JSVGRasterizer reflectively using Class.forName() and also calling all relevant methods reflectively (or a method handle to improve performance).
This way a special interface to implement would not be necessary at all, but it would also be a bit more 'bare-metal'.
Edit: Thinking about this again, this wouldn't save much since e.g. the ImageData type is still required, which leads to the same problem again.

Yet another option would be to avoid a new o.e.swt.svg bundle/fragment and add all code to interact with JSVG, which is not that much, directly into o.e.swt respectivly it's native fragments and make the dependency to JSVG optional and be prepared for it's absence! This way one would only have to add jsvg to an application/product for OSGi-runtimes and plain Java apps and not also o.e.swt.svg. Hopefully it has OSGi metadata soon:

• [Build] Generate OSGi metadata for JSVG weisJ/jsvg#107

Of course this would more or less make it impossible to supply alternative rasterizer. But I cannot tell if it's realistic that custom ones will be supplied or not and we are just over-engineering here?
Of course it shouldn't be integrated to tightly, so that an exchange in the future is feasible if necessary. But that's maybe part of the discussion of the very first point of this comment.

For some more clarifiation on using a fragment: If we are not mistaken, to provide a fragment with a concrete rasterizer implementation, the rasterizer interface needs to be placed directly in the SWT host bundle, not in some common package that is mapped into all the native fragments like done for all the other SWT code.

This is not true, all fragments are merged with the host and therefore you have one big class space, it might be that the IDE (or Tycho) currently have some limitations in this regard but we could/should fix that instead of making things harder than required.

A ServiceLoader approach seem suitable also here for me, no need to complicate the thing with OSGi SPI, this is only for applications that are not aware of OSGi at all and this is not the case here and also not needed.

Yes, absolutely. I suggest we discuss this in a separate issue, in parallel to this PR.

I agree. I created this discussion for this topic.

I just realised that cocoa and gtk not only have different implementation for ImageLoader but also for the Image class. I can of course also implement my changes there but right now I can't test the changes.